Externalize secondary storage capacity threshold by GutoVeronezi · Pull Request #4790 · apache/cloudstack

GutoVeronezi · 2021-03-10T17:28:29Z

Description

On StatsCollector, it limits secondary storages capacity to 90%; therefore, operators do not have option to choose which threshold they want/need.

This PR intends to externalize the secondary storage capacity threshold configuration on StatsCollector (secondary.storage.capacity.threshold), validating if it is between 0 and 100% (0.00 and 1.00).

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functioanlity)
Cleanup (Code refactoring and cleanup, that may add test cases)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

How Has This Been Tested?

It has been tested locally on a test lab.

I enabled a zone;
I created a template and observed the log. It will log messages according to the configuration set.

DaanHoogland · 2021-03-11T09:52:16Z

server/src/main/java/com/cloud/server/StatsCollector.java

+        s_logger.warn(String.format("Invalid [%s] configuration: value set [%s] is [%s]. Assuming %s as secondary storage capacity threshold.",
+                                    secondaryStorageCapacityThreshold.key(),
+                                    thresholdConfig,
+                                    thresholdConfig < MIN_STORAGE_SECONDARY_CAPACITY_THRESHOLD ? String.format("lower than '%s'", MIN_STORAGE_SECONDARY_CAPACITY_THRESHOLD) : String.format("bigger than '%s'", MAX_STORAGE_SECONDARY_CAPACITY_THRESHOLD),
+                                    MAX_STORAGE_SECONDARY_CAPACITY_THRESHOLD));
+        return MAX_STORAGE_SECONDARY_CAPACITY_THRESHOLD;


makes you wish for a bounds checker on entry, 👍

DaanHoogland · 2021-03-11T09:54:16Z

server/src/main/java/com/cloud/server/StatsCollector.java

+                                    thresholdConfig,
+                                    thresholdConfig < MIN_STORAGE_SECONDARY_CAPACITY_THRESHOLD ? String.format("lower than '%s'", MIN_STORAGE_SECONDARY_CAPACITY_THRESHOLD) : String.format("bigger than '%s'", MAX_STORAGE_SECONDARY_CAPACITY_THRESHOLD),
+                                    MAX_STORAGE_SECONDARY_CAPACITY_THRESHOLD));
+        return MAX_STORAGE_SECONDARY_CAPACITY_THRESHOLD;


i think we should revert to default, i.e. 0.9

I thought about it too and came to a possible resolution:

When the config is bigger than max, I think we should return max (as it is top limit);

When the config is lower than min, then we use default;

What do you think?

sound like a plan

@GutoVeronezi these value limits are controlled/validated in the configuration manager. you can add this new config to 'weightBasedParametersForValidation' in configuration manager. check here:

cloudstack/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java

Line 479 in a0d9ace

private void weightBasedParametersForValidation() {

you can simply return threshold config value here.

DaanHoogland

cltgm

GabrielBrascher

Code LGTM. I did not test it, though.

GabrielBrascher · 2021-04-20T14:39:06Z

@blueorangutan package

blueorangutan · 2021-04-20T14:40:02Z

@GabrielBrascher a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-04-20T15:02:25Z

Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 440

GutoVeronezi · 2021-05-17T14:28:28Z

@DaanHoogland @GabrielBrascher is there anything else to do?

yadvr · 2021-05-25T13:50:33Z

@blueorangutan package

blueorangutan · 2021-05-25T13:51:03Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-05-25T14:43:06Z

Packaging result: ✔️ centos7 ✔️ centos8 ✔️ debian. SL-JID 88

sureshanaparti · 2021-05-26T05:47:49Z

server/src/main/java/com/cloud/server/StatsCollector.java

            return true;
        }
+
+        s_logger.warn(String.format("Image storage [%s] has not enough capacity. Capacity: total=[%s], used=[%s], threshold=[%s%%].", imageStoreId,readableTotalCapacity, readableUsedCapacity, threshold * 100));


Suggested change

s_logger.warn(String.format("Image storage [%s] has not enough capacity. Capacity: total=[%s], used=[%s], threshold=[%s%%].", imageStoreId,readableTotalCapacity, readableUsedCapacity, threshold * 100));

s_logger.warn(String.format("Image storage [%s] has not enough capacity.", imageStoreId));

capacity/used bytes and threshold already logged above, may not be required here.

I think it could be interesting to log here too to not depend on another log line; Also we could lose the information in some situations.

sureshanaparti · 2021-05-26T06:34:56Z

server/src/main/java/com/cloud/server/StatsCollector.java

            "URI to send StatsCollector statistics to. The collector is defined on the URI scheme. Example: graphite://graphite-hostaddress:port or influxdb://influxdb-hostaddress/dbname. Note that the port is optional, if not added the default port for the respective collector (graphite or influxdb) will be used. Additionally, the database name '/dbname' is  also optional; default db name is 'cloudstack'. You must create and configure the database if using influxdb.",
            true);
+    private static final ConfigKey<Double> secondaryStorageCapacityThreshold = new ConfigKey<Double>("Advanced", Double.class, "secondary.storage.capacity.threshold", "0.90",
+            "Secondary storage capacity threshold (1 = 100%).", true);


@GutoVeronezi can you move this config to 'CapacityManager' ? and use it wherever applicable.

also, update StorageOrchestrator class with this config, it is hardcoded here:

cloudstack/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/StorageOrchestrator.java

Line 104 in a0d9ace

private double imageStoreCapacityThreshold = 0.90;

sureshanaparti · 2021-05-26T06:37:12Z

server/src/main/java/com/cloud/server/StatsCollector.java

    private static final String INFLUXDB_HOST_MEASUREMENT = "host_stats";
    private static final String INFLUXDB_VM_MEASUREMENT = "vm_stats";

+    private static final double MIN_STORAGE_SECONDARY_CAPACITY_THRESHOLD = 0.00;


Suggested change

private static final double MIN_STORAGE_SECONDARY_CAPACITY_THRESHOLD = 0.00;

private static final double MIN_SECONDARY_STORAGE_CAPACITY_THRESHOLD = 0.00;

Note: this is not required, If you use configuration framework.

sureshanaparti · 2021-05-26T06:37:30Z

server/src/main/java/com/cloud/server/StatsCollector.java

    private static final String INFLUXDB_VM_MEASUREMENT = "vm_stats";

+    private static final double MIN_STORAGE_SECONDARY_CAPACITY_THRESHOLD = 0.00;
+    private static final double MAX_STORAGE_SECONDARY_CAPACITY_THRESHOLD = 1.00;


Suggested change

private static final double MAX_STORAGE_SECONDARY_CAPACITY_THRESHOLD = 1.00;

private static final double MAX_SECONDARY_STORAGE_CAPACITY_THRESHOLD = 1.00;

Note: this is not required, If you use configuration framework.

sureshanaparti · 2021-05-26T06:54:02Z

server/src/main/java/com/cloud/server/StatsCollector.java

    private static final ConfigKey<String> statsOutputUri = new ConfigKey<String>("Advanced", String.class, "stats.output.uri", "",
            "URI to send StatsCollector statistics to. The collector is defined on the URI scheme. Example: graphite://graphite-hostaddress:port or influxdb://influxdb-hostaddress/dbname. Note that the port is optional, if not added the default port for the respective collector (graphite or influxdb) will be used. Additionally, the database name '/dbname' is  also optional; default db name is 'cloudstack'. You must create and configure the database if using influxdb.",
            true);
+    private static final ConfigKey<Double> secondaryStorageCapacityThreshold = new ConfigKey<Double>("Advanced", Double.class, "secondary.storage.capacity.threshold", "0.90",


The description for most of the percentage based configs clear indicate that values should be between 0 and 1, and same is being validated in the configuration manager. can you search code with string 'Percentage (as a value between 0 and 1) ' and update the config description as appropriate. Thanks.

GutoVeronezi · 2021-05-31T15:52:56Z

@sureshanaparti Thank you for the review.

As you suggested, I added the config to the validation in ConfigurationManagerImpl. However, I had to change the config type from Double to Float (not that it is a problem) because the validation only works on Float configs.
I tested some of they and realize that those with Double type are not validated. I think we need some refactors in this validation, in another PR. What do you think?

DaanHoogland · 2021-06-10T14:43:20Z

@sureshanaparti are you satisfied?
do we need more testing? cc @borisstoyanov @vladimirpetrov

GutoVeronezi · 2021-06-21T13:57:36Z

@sureshanaparti @DaanHoogland @GabrielBrascher is there anything else to do?

GutoVeronezi · 2021-07-05T12:17:30Z

@sureshanaparti @DaanHoogland @GabrielBrascher is there anything else to do?

sureshanaparti · 2021-07-13T07:02:55Z

@sureshanaparti Thank you for the review.

As you suggested, I added the config to the validation in ConfigurationManagerImpl. However, I had to change the config type from Double to Float (not that it is a problem) because the validation only works on Float configs.
I tested some of they and realize that those with Double type are not validated. I think we need some refactors in this validation, in another PR. What do you think?

@GutoVeronezi agree, these validations can be refactored in another PR.

sureshanaparti · 2021-07-13T07:06:15Z

@blueorangutan package

blueorangutan · 2021-07-13T07:07:04Z

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2021-07-13T07:32:01Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian. SL-JID 533

sureshanaparti · 2021-07-13T07:37:42Z

@blueorangutan test

blueorangutan · 2021-07-13T07:38:03Z

@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2021-07-13T21:31:09Z

Trillian test result (tid-1250)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 47948 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr4790-t1250-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_kubernetes_clusters.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 87 look OK, 1 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_01_invalid_upgrade_kubernetes_cluster	`Failure`	3615.86	test_kubernetes_clusters.py
test_02_deploy_and_upgrade_kubernetes_cluster	`Failure`	3616.17	test_kubernetes_clusters.py
test_03_deploy_and_scale_kubernetes_cluster	`Failure`	0.06	test_kubernetes_clusters.py
test_04_basic_lifecycle_kubernetes_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_05_delete_kubernetes_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_07_deploy_kubernetes_ha_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_08_deploy_and_upgrade_kubernetes_ha_cluster	`Failure`	0.04	test_kubernetes_clusters.py
test_09_delete_kubernetes_ha_cluster	`Failure`	0.04	test_kubernetes_clusters.py
ContextSuite context=TestKubernetesCluster>:teardown	`Error`	94.79	test_kubernetes_clusters.py

Externalize secondary storage capacity threshold

c6649da

DaanHoogland reviewed Mar 11, 2021

View reviewed changes

GutoVeronezi requested a review from DaanHoogland March 11, 2021 18:24

Use default value as threshold when config value is lower than 0.0

3ec2ed6

GutoVeronezi force-pushed the externalize-secondary-storage-capacity-threshold branch from 3fc2702 to 3ec2ed6 Compare March 11, 2021 18:36

DaanHoogland approved these changes Mar 12, 2021

View reviewed changes

GabrielBrascher approved these changes Apr 20, 2021

View reviewed changes

DaanHoogland added component:agent Severity:Minor status:needs-testing type:enhancement labels Apr 23, 2021

sureshanaparti reviewed May 26, 2021

View reviewed changes

Daniel Augusto Veronezi Salvador added 4 commits May 31, 2021 11:38

Move config to CapacityManager

da074f0

Validate config in CapacityManagerImpl

7330f2a

Use config in StorageOrchestrator

a0fabd6

Change config description

69a7de3

Remove unused import

db8362d

sureshanaparti approved these changes Jul 13, 2021

View reviewed changes

DaanHoogland added this to the 4.16.0.0 milestone Jul 16, 2021

DaanHoogland merged commit cbe380a into apache:main Jul 16, 2021

DaanHoogland removed the status:needs-testing label Jan 28, 2026

	s_logger.warn(String.format("Image storage [%s] has not enough capacity. Capacity: total=[%s], used=[%s], threshold=[%s%%].", imageStoreId,readableTotalCapacity, readableUsedCapacity, threshold * 100));
	s_logger.warn(String.format("Image storage [%s] has not enough capacity.", imageStoreId));

	private static final double MIN_STORAGE_SECONDARY_CAPACITY_THRESHOLD = 0.00;
	private static final double MIN_SECONDARY_STORAGE_CAPACITY_THRESHOLD = 0.00;

	private static final double MAX_STORAGE_SECONDARY_CAPACITY_THRESHOLD = 1.00;
	private static final double MAX_SECONDARY_STORAGE_CAPACITY_THRESHOLD = 1.00;

Conversation

GutoVeronezi commented Mar 10, 2021

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

How Has This Been Tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sureshanaparti May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

GabrielBrascher commented Apr 20, 2021

Uh oh!

blueorangutan commented Apr 20, 2021

Uh oh!

blueorangutan commented Apr 20, 2021

Uh oh!

GutoVeronezi commented May 17, 2021

Uh oh!

yadvr commented May 25, 2021

Uh oh!

blueorangutan commented May 25, 2021

Uh oh!

blueorangutan commented May 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sureshanaparti May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sureshanaparti May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GutoVeronezi commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaanHoogland commented Jun 10, 2021

Uh oh!

GutoVeronezi commented Jun 21, 2021

Uh oh!

GutoVeronezi commented Jul 5, 2021

Uh oh!

sureshanaparti commented Jul 13, 2021

Uh oh!

sureshanaparti commented Jul 13, 2021

Uh oh!

blueorangutan commented Jul 13, 2021

Uh oh!

blueorangutan commented Jul 13, 2021

Uh oh!

sureshanaparti commented Jul 13, 2021

sureshanaparti May 26, 2021 •

edited

Loading

sureshanaparti May 26, 2021 •

edited

Loading

sureshanaparti May 26, 2021 •

edited

Loading

GutoVeronezi commented May 31, 2021 •

edited

Loading